Skip to content

Ensure reading request body does not drain the input stream if stream is resettable#1537

Merged
Ndiritu merged 5 commits into
mainfrom
fix/request-body-write
Aug 22, 2024
Merged

Ensure reading request body does not drain the input stream if stream is resettable#1537
Ndiritu merged 5 commits into
mainfrom
fix/request-body-write

Conversation

@Ndiritu

@Ndiritu Ndiritu commented Aug 19, 2024

Copy link
Copy Markdown
Contributor

We currently override the Request body's writeTo method which was not resetting the input stream after the body is written e.g. by a logging interceptor. The network call would find an empty request body.

This PR determines if the request body is resettable & configures isOneShot() to prevent non-resettable streams from being drained by logging middleware.

closes microsoftgraph/msgraph-sdk-java#2037

@Ndiritu
Ndiritu force-pushed the fix/request-body-write branch from 418176a to cabb0fd Compare August 19, 2024 15:33
Comment thread components/http/okHttp/gradle/dependencies.gradle Outdated
@Ndiritu
Ndiritu force-pushed the fix/request-body-write branch 2 times, most recently from a777477 to 67b084f Compare August 21, 2024 18:06
@baywet

baywet commented Aug 21, 2024

Copy link
Copy Markdown
Member

for others who might be following along:

  • the logging interceptor is draining the stream
  • when the transport picks it up, it's already drained
  • the main reason it's most likely not doing a buffered copy is because they instead leverage the "isOneShot" method on the body to avoid draining the request body if it shouldn't.
  • Our implementation needs to implement that method in the request adapter, based on whether the underlying stream is markable or not
  • additionally, when the underlying stream is markable, in the writeTo method, we need to reset/mark it to zero
  • This option is much better in terms of performance than buffering everything for this edge case.
  • It's also much better in terms of UX than giving a setting on the request adapter to buffer ONLY when requested.

@Ndiritu
Ndiritu force-pushed the fix/request-body-write branch from fd88bfa to 6c2d170 Compare August 22, 2024 12:02
@Ndiritu Ndiritu changed the title Update request body write to retain inputstream buffer Ensure reading request body does not drain the input stream if stream is not resettable Aug 22, 2024
@Ndiritu
Ndiritu marked this pull request as ready for review August 22, 2024 12:06
@Ndiritu
Ndiritu requested a review from a team as a code owner August 22, 2024 12:06
@Ndiritu
Ndiritu requested a review from baywet August 22, 2024 12:06
baywet
baywet previously approved these changes Aug 22, 2024

@baywet baywet left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for making the changes!
A version bump, a changelog entry and we should be good to go!

baywet
baywet previously approved these changes Aug 22, 2024
@Ndiritu

Ndiritu commented Aug 22, 2024

Copy link
Copy Markdown
Contributor Author

Thank you for making the changes! A version bump, a changelog entry and we should be good to go!

Ah no release please yet :)
On it.

@Ndiritu
Ndiritu force-pushed the fix/request-body-write branch from fb23874 to 2c3191f Compare August 22, 2024 12:29
@Ndiritu
Ndiritu requested a review from baywet August 22, 2024 12:29
@Ndiritu
Ndiritu enabled auto-merge August 22, 2024 12:29
@sonarqubecloud

Copy link
Copy Markdown

@Ndiritu
Ndiritu merged commit 6b5fde9 into main Aug 22, 2024
@Ndiritu
Ndiritu deleted the fix/request-body-write branch August 22, 2024 12:40
@baywet

baywet commented Aug 22, 2024

Copy link
Copy Markdown
Member

@Ndiritu the tag also needs to be manually created on this repo for the time being. I'll let you take care of that :)

@mkomko

mkomko commented Aug 22, 2024

Copy link
Copy Markdown

@Ndiritu @baywet Thanks a lot guys! ❤️

@Ndiritu Ndiritu changed the title Ensure reading request body does not drain the input stream if stream is not resettable Ensure reading request body does not drain the input stream if stream is resettable Dec 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

0-byte body on sdk 6.12 with android when using HttpLoggingInterceptor

3 participants